Repository navigation
Throw the named error type from JSError.Throw*Error(string) - #511
René Larch (r-Larch) wants to merge 3 commits into
Conversation
The string buffer is rented from ArrayPool<byte>.Shared on .NET, which is not zeroed, so the byte reserved for each terminator could hold stale data and native code read argv entries past their end. Same fix as microsoft#482 (fixes microsoft#481); needed here because the new tests dirty the shared pool before the Node.js platform is created. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PooledBuffer.FromStringUtf8 and FromSpanUtf8 reserved a byte for the
terminator but never wrote it. On .NET the array comes from
ArrayPool<byte>.Shared, which is not zeroed, so native functions that take a
null-terminated const char* without a length read stale bytes past the string
(or past the array): napi_throw_error / napi_throw_type_error /
napi_throw_range_error / node_api_throw_syntax_error (code and msg),
node_embedding_runtime_config_add_module and
node_embedding_last_error_message_set. For example JSError.ThrowError("x")
produced a JS error message of "x" followed by garbage, and a linked module
could be registered under a corrupted name.
- Write the terminator explicitly in both helpers.
- Encode an empty string as just the terminator instead of returning
PooledBuffer.Empty, which pinned to a null pointer (ThrowError("") passed
msg = NULL, so napi_throw_error failed and nothing was thrown). A null
string still maps to a null pointer.
- Return the module-name buffer to the pool in
EmbeddingRuntimeConfigAddModule (Node.js copies the name).
Fixes microsoft#508.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JSError.ThrowTypeError, ThrowRangeError and ThrowSyntaxError (with and without a code) all called JSRuntime.ThrowError, so JS received a plain Error instead of a TypeError / RangeError / SyntaxError. Call the matching runtime method instead. Fixes microsoft#509. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
René Larch (@r-Larch) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
René Larch (@r-Larch) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused fixes match the reported defects and include targeted regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes #509 by making typed JSError helpers throw the matching JavaScript error type. Includes the UTF-8 buffer fixes stacked from #510.
Changes:
- Routes typed throw helpers to their matching runtime methods.
- Explicitly terminates UTF-8 strings and disposes the module-name buffer.
- Adds regression tests for error types and buffer handling.
| File | Description |
|---|---|
| test/Utf8BufferTests.cs | Tests UTF-8 termination and null handling. |
| test/TestUtils.cs | Adds a helper to dirty pooled buffers. |
| test/NodejsEmbeddingTests.cs | Tests thrown error types, messages, and codes. |
| src/NodeApi/Runtime/Utf8StringArray.cs | Writes each string’s terminator explicitly. |
| src/NodeApi/Runtime/PooledBuffer.cs | Terminates UTF-8 buffers and distinguishes empty from null. |
| src/NodeApi/Runtime/NodejsRuntime.Embedding.cs | Disposes the module-name buffer. |
| src/NodeApi/JSError.cs | Calls the matching typed runtime throw methods. |
Review effort: Balanced
|
@microsoft-github-policy-service agree |
Fixes #509.
JSError.ThrowTypeError,ThrowRangeErrorandThrowSyntaxError(with and withoutcode) all calledJSRuntime.ThrowError, so JS received a plainError. They now call the matching runtime method (napi_throw_type_error,napi_throw_range_error,node_api_throw_syntax_error).Tests
NodejsEmbeddingTests.ThrowTypedErrorchecksname,instanceof,messageandcodeforError,TypeError,RangeErrorandSyntaxError, with and without a code. The non-Errorcases fail onmain.Local run (Windows x64, .NET SDK 10.0.402): net9.0 134/135 and net8.0 134/135. On net10.0 everything passed except
napi-dotnet/overloads(hosted and AOT), which fails the same way onmain. I could not run net472 locally.🤖 Generated with Claude Code